Skip to content

Fix second element of forms starting with 'def' being highlighted as function name #632

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 3, 2022

Conversation

OknoLombarda
Copy link
Contributor

Follow up to #630

Forgot to make function definition regexp only look for defn forms. This PR will fix such behaviour:
image


Before submitting a PR mark the checkboxes for the items you've done (if you
think a checkbox does not apply, then leave it unchecked):

  • The commits are consistent with our contribution guidelines.
  • You've added tests (if possible) to cover your change(s). Bugfix, indentation, and font-lock tests are extremely important!
  • You've run M-x checkdoc and fixed any warnings in the code you've written.
  • You've updated the changelog (if adding/changing user-visible functionality).
  • You've updated the readme (if adding/changing user-visible functionality).

Thanks!

@OknoLombarda
Copy link
Contributor Author

OknoLombarda commented Sep 3, 2022

Maybe it's better to also remove optional namespace from regexp? I mean, to allow only clojure.core

(,(concat "(\\(?:" clojure--sym-regexp "/\\)?"

@OknoLombarda OknoLombarda changed the title Fix second element of forms starting with 'def' being highlighted as function names Fix second element of forms starting with 'def' being highlighted as function name Sep 3, 2022
@bbatsov
Copy link
Member

bbatsov commented Sep 3, 2022

Yeah, that'd be better. It used to accept all namespaces just because it could be anything.

@OknoLombarda
Copy link
Contributor Author

Done

@@ -780,13 +780,13 @@ DESCRIPTION is the description of the spec."
(2 4 font-lock-type-face)
(5 5 nil)
(6 9 font-lock-keyword-face)
(11 13 font-lock-function-name-face))
(11 13 nil))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this example should be updated to use clojure-core, otherwise it's kind of pointless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, makes sense. Done


("(clo/defrecord foo nil)"
(2 4 font-lock-type-face)
(5 5 nil)
(6 14 font-lock-keyword-face)
(16 18 font-lock-function-name-face))
(16 18 nil))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me this example needs to be updated as well to use clojure.core, as I doubt they would be a different ns for defrecord. Also we should probably make defrecord and deftype use font-lock-type-face for the name of the type.

Copy link
Contributor Author

@OknoLombarda OknoLombarda Sep 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced namespace with clojure.core.

Also we should probably make defrecord and deftype use font-lock-type-face for the name of the type.

We already do, I didn't change that. I changed the test to check if this face is used

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't 16-18 the name of the type foo? It seems it has no font-locking now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see this wasn't working because of the wrong namespace. My bad.

@bbatsov
Copy link
Member

bbatsov commented Sep 3, 2022

It's good we have so many tests - they helped me spot two weird behaviors currently that it would be nice to address.

@bbatsov bbatsov merged commit 24ee368 into clojure-emacs:master Sep 3, 2022
@bbatsov
Copy link
Member

bbatsov commented Sep 3, 2022

Looks good now. Thanks!

@OknoLombarda OknoLombarda deleted the fix-forms-with-def branch September 3, 2022 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants